fix(sdk): *Secure key helpers don't hash GetKeyResponse key material - #1066
Open
0xkurious wants to merge 1 commit into
Open
fix(sdk): *Secure key helpers don't hash GetKeyResponse key material#10660xkurious wants to merge 1 commit into
0xkurious wants to merge 1 commit into
Conversation
toViemAccountSecure, toKeypairSecure (JS) and to_account_secure, to_keypair_secure (Python) all document "applies SHA256 hashing to the complete key material for enhanced security" -- but only the legacy GetTlsKeyResponse branch actually did this. The GetKeyResponse branch (the path client.getKey() + these helpers, i.e. exactly what the README recommends) fell straight through to the raw derived key, identical to the deprecated non-secure variant. Found while wiring a TEE-attested signer against a live CVM: the security guarantee these functions are named and documented for silently did not apply to the primary documented usage. BREAKING: anyone currently using toViemAccountSecure/toKeypairSecure/ to_account_secure/to_keypair_secure with a GetKeyResponse (not GetTlsKeyResponse) will get a different address/keypair after this fix, since the key material is now actually hashed as documented. Flagging for maintainers to decide on versioning/changelog treatment. Added regression tests in all four modules asserting the *Secure variant produces a different address/pubkey than the legacy variant for the same GetKeyResponse -- this is exactly the assertion that would have caught the bug.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
toViemAccountSecure/toKeypairSecure(JS) andto_account_secure/to_keypair_secure(Python) all document:But only the legacy
GetTlsKeyResponsebranch actually does this. TheGetKeyResponsebranch — i.e.client.getKey()followed by the*Securehelper, which is exactly the pattern the README recommends — falls straight through to the raw derived key, producing output byte-identical to the deprecated non-secure variant (toViemAccount/toKeypair/to_account/to_keypair).So the documented security guarantee silently does not apply to the primary, documented usage path. Found this while wiring a TEE-attested signer against a live CVM (deriving an EVM address from
getKey()to bind into a quote'sREPORTDATA) and noticingtoViemAccountSecure(key)produced the exact same address as the deprecated helper.Root cause
This looks like a regression against the intent of #197, which introduced the
*Securefamily specifically to apply SHA256 across "complete key material" for both response types. TheGetTlsKeyResponsebranch got the hash; theGetKeyResponsebranch didn't.Same pattern in
sdk/js/src/solana.ts,sdk/python/src/dstack_sdk/ethereum.py, andsdk/python/src/dstack_sdk/solana.py.Fix
Apply
sha256()tokeyResponse.key(JS) /get_key_response.decode_key()(Python) in theGetKeyResponsebranch of all four*Securefunctions, matching the existingGetTlsKeyResponsebranch and the documented behavior.Anyone currently calling
toViemAccountSecure/toKeypairSecure/to_account_secure/to_keypair_securewith aGetKeyResponse(the common case — not the deprecated TLS-key path) will get a different address/keypair after this fix, since the key material is now actually hashed as documented. The underlyinggetKey()derivation and the raw key itself are unaffected — only what these four wrapper functions do with it changes.I don't have visibility into how many consumers rely on the current (unhashed) behavior, so I'm flagging this explicitly rather than deciding on versioning myself. Options I can see:
GetKeyResponsecallers matters more than closing the gap, an alternative is to fix the docstring instead of the code (state plainly that only the TLS path is hashed) — happy to switch the PR to that if preferred.I went with fixing the implementation because a function literally named
*Securesilently not doing what its docstring promises seems like the worse failure mode, but the versioning/migration call is yours.Testing
Added a regression test to each of the four modules asserting the
*Securevariant produces a different address/pubkey than the legacy variant for the sameGetKeyResponse— this is exactly the assertion that would have caught this bug originally. All pre-existing tests still pass (verified locally against thephala simulator); a handful of unrelated pre-existing failures insdk/js/src/__tests__/index.test.ts(secp256k1_prehashed validation) reproduce identically on unmodifiedmainand are not touched by this PR.sdk/js:npx vitest run src/__tests__/viem.test.ts src/__tests__/solana.test.ts→ 14/14 passedsdk/python:pytest tests/test_ethereum.py tests/test_solana.py→ 14/14 passed